module,win: fix long path resolve by StefanStojanovic · Pull Request #51097 · nodejs/node

StefanStojanovic · 2023-12-08T13:28:00Z

As reported here, there is a problem loading modules from long paths on Windows. This change fixes it by adding the required \\?\ prefix, with which, resolving long paths will work regardless of whether the LongPathsEnabled value is set or not.

Fixes: #50753

anonrig

Can we add a test so we don't regress?

richardlau · 2023-12-08T16:16:45Z

FYI @nodejs/loaders @nodejs/fs

StefanStojanovic · 2023-12-08T18:51:38Z

Can we add a test so we don't regress?

Of course. I'll add it next week.

StefanStojanovic · 2023-12-15T10:01:49Z

Can we add a test so we don't regress?

Test's added.

joyeecheung

This looks like a regression from #50322, probably from not porting the logic in path.toNamespacedPath() properly

node/lib/path.js

Lines 632 to 648 in b7def8e

    
           if (StringPrototypeCharCodeAt(resolvedPath, 0) === CHAR_BACKWARD_SLASH) { 
        
             // Possible UNC root 
        
             if (StringPrototypeCharCodeAt(resolvedPath, 1) === CHAR_BACKWARD_SLASH) { 
        
               const code = StringPrototypeCharCodeAt(resolvedPath, 2); 
        
               if (code !== CHAR_QUESTION_MARK && code !== CHAR_DOT) { 
        
                 // Matched non-long UNC root, convert the path to a long UNC path 
        
                 return `\\\\?\\UNC\\${StringPrototypeSlice(resolvedPath, 2)}`; 
        
               } 
        
             } 
        
           } else if ( 
        
             isWindowsDeviceRoot(StringPrototypeCharCodeAt(resolvedPath, 0)) && 
        
             StringPrototypeCharCodeAt(resolvedPath, 1) === CHAR_COLON && 
        
             StringPrototypeCharCodeAt(resolvedPath, 2) === CHAR_BACKWARD_SLASH 
        
           ) { 
        
             // Matched device root, convert the path to a long UNC path 
        
             return `\\\\?\\${resolvedPath}`; 
        
           }

We should probably port the entire logic over (with a helper that's named something like ToNamespacedPath declared in either util.h or node_internals.h)

StefanStojanovic · 2024-02-08T11:49:11Z

@joyeecheung do you think this can land as a fix with a regression test added and then a follow-up PR can be opened for porting path.toNamespacedPath() logic and using it in here, but potentially in other places missing it as well?

joyeecheung · 2024-02-08T13:27:58Z

Yes, with a TODO/FIXME comment.

StefanStojanovic · 2024-02-09T09:54:47Z

@joyeecheung LGTY now?

nodejs-github-bot · 2024-02-20T22:55:48Z

CI: https://ci.nodejs.org/job/node-test-pull-request/57224/

nodejs-github-bot · 2024-02-29T11:17:44Z

CI: https://ci.nodejs.org/job/node-test-pull-request/57499/

nodejs-github-bot · 2024-02-29T11:44:15Z

CI: https://ci.nodejs.org/job/node-test-pull-request/57500/

Fixes: nodejs#50753

nodejs-github-bot · 2024-03-01T00:41:05Z

CI: https://ci.nodejs.org/job/node-test-pull-request/57517/

nodejs-github-bot · 2024-03-19T21:01:28Z

CI: https://ci.nodejs.org/job/node-test-pull-request/57848/

StefanStojanovic · 2024-04-01T10:55:50Z

@joyeecheung can you add the commit-queue label to try to land this again? Last time it failed to do so...

nodejs-github-bot · 2024-04-01T17:51:22Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58036/

karlhorky · 2024-04-03T20:23:57Z

@StefanStojanovic @joyeecheung any chance of getting this released in one of the next patch or minor releases?

It's been a while since my original issue in Nov 2023 and users keep running into this (eg. with pnpm) and commenting that it's an open bug but the PR hasn't been merged yet.

nodejs-github-bot · 2024-04-08T07:16:34Z

Commit Queue failed

- Loading data for nodejs/node/pull/51097
✔  Done loading data for nodejs/node/pull/51097
----------------------------------- PR info ------------------------------------
Title      module,win: fix long path resolve (#51097)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     StefanStojanovic:mefi-resolve-long-path -> nodejs:main
Labels     c++, fs, needs-ci
Commits    1
 - module,win: fix long path resolve
Committers 1
 - StefanStojanovic 
PR-URL: https://github.com/nodejs/node/pull/51097
Fixes: https://github.com/nodejs/node/issues/50753
Reviewed-By: Joyee Cheung 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/51097
Fixes: https://github.com/nodejs/node/issues/50753
Reviewed-By: Joyee Cheung 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - module,win: fix long path resolve
   ℹ  This PR was created on Fri, 08 Dec 2023 13:28:00 GMT
   ✔  Approvals: 1
   ✔  - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/51097#pullrequestreview-1891732903
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-04-01T17:51:22Z: https://ci.nodejs.org/job/node-test-pull-request/58036/
- Querying data for job/node-test-pull-request/58036/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/8596048782

StefanStojanovic · 2024-04-08T07:25:36Z

Commit Queue failed

Loading data for module,win: fix long path resolve #51097
✔ Done loading data for module,win: fix long path resolve #51097
----------------------------------- PR info ------------------------------------
Title module,win: fix long path resolve (module,win: fix long path resolve #51097)
⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch StefanStojanovic:mefi-resolve-long-path -> nodejs:main
Labels c++, fs, needs-ci
Commits 1

module,win: fix long path resolve
Committers 1

StefanStojanovic
PR-URL: module,win: fix long path resolve #51097
Fixes: Long node_modules paths cannot be found on Windows when LongPathsEnabled enabled #50753
Reviewed-By: Joyee Cheung
------------------------------ Generated metadata ------------------------------
PR-URL: module,win: fix long path resolve #51097
Fixes: Long node_modules paths cannot be found on Windows when LongPathsEnabled enabled #50753
Reviewed-By: Joyee Cheung

⚠ Commits were pushed since the last approving review:
⚠ - module,win: fix long path resolve
ℹ This PR was created on Fri, 08 Dec 2023 13:28:00 GMT
✔ Approvals: 1
✔ - Joyee Cheung (@joyeecheung) (TSC): #51097 (review)
✔ Last GitHub CI successful
ℹ Last Full PR CI on 2024-04-01T17:51:22Z: https://ci.nodejs.org/job/node-test-pull-request/58036/

Querying data for job/node-test-pull-request/58036/
✔ Last Jenkins CI successful

✔ Aborted git node land session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/8596048782

@joyeecheung do you see why this couldn't land from this log?

nodejs-github-bot · 2024-04-08T08:09:06Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58206/

nodejs-github-bot · 2024-04-08T14:37:47Z

Landed in 45f0dd0

karlhorky · 2024-05-02T17:00:44Z

It looks like the Node.js v22 PR contains the fix "module,win: fix long path resolve (by @StefanStojanovic) #51097":

2024-04-24, Version 22.0.0 (Current) #52505

So maybe Node.js has support for long paths on Windows now!

(make sure that you have LongPathsEnabled set to true on the Windows machine you test on)

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Dec 8, 2023

anonrig reviewed Dec 8, 2023

View reviewed changes

StefanStojanovic force-pushed the mefi-resolve-long-path branch from e74bcf7 to ec2ea14 Compare December 15, 2023 10:00

joyeecheung reviewed Dec 18, 2023

View reviewed changes

StefanStojanovic force-pushed the mefi-resolve-long-path branch from ec2ea14 to 3f957e8 Compare February 9, 2024 09:54

StefanStojanovic force-pushed the mefi-resolve-long-path branch 3 times, most recently from e72e49a to 4e651a1 Compare February 9, 2024 13:22

joyeecheung approved these changes Feb 20, 2024

View reviewed changes

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 20, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 20, 2024

RexSkz mentioned this pull request Feb 28, 2024

How commit all patches at onece? pnpm/pnpm#7291

Open

1 task

module,win: fix long path resolve

93dc8cb

Fixes: nodejs#50753

StefanStojanovic force-pushed the mefi-resolve-long-path branch from 4e651a1 to 93dc8cb Compare February 29, 2024 20:39

aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 1, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 1, 2024

joyeecheung added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 1, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 1, 2024

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 1, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 1, 2024

karlhorky mentioned this pull request Apr 5, 2024

Bug: Long node_modules plugin paths cannot be found on Windows eslint/eslint#17763

Closed

1 task

zkochan mentioned this pull request Apr 7, 2024

feat(dlx): cache pnpm/pnpm#7835

Merged

11 tasks

joyeecheung added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 8, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 8, 2024

joyeecheung approved these changes Apr 8, 2024

View reviewed changes

joyeecheung added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Apr 8, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 8, 2024

joyeecheung added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 8, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 8, 2024

nodejs-github-bot merged commit 45f0dd0 into nodejs:main Apr 8, 2024

RafaelGSS mentioned this pull request Apr 15, 2024

2024-04-24, Version 22.0.0 (Current) #52505

Merged

karlhorky mentioned this pull request Apr 28, 2024

Flattening feature creates lengthy folder structures pnpm/pnpm#6830

Open

This was referenced May 2, 2024

Long node_modules paths cannot be found on Windows when LongPathsEnabled enabled #50753

Closed

Upgrade to Node.js v22 karlhorky/node-js-max_path-windows-bug#1

Open

marco-ippolito added the backport-blocked-v20.x PRs that should land on the v20.x-staging branch but are blocked by another PR's pending backport. label May 21, 2024

karlhorky mentioned this pull request Sep 9, 2024

Remove Windows long path workaround upleveled/preflight#572

Merged

joyeecheung mentioned this pull request Jan 23, 2025

[v20.x backport] src: move package_json_reader cache to c++ #56590

Closed

	if (StringPrototypeCharCodeAt(resolvedPath, 0) === CHAR_BACKWARD_SLASH) {
	// Possible UNC root
	if (StringPrototypeCharCodeAt(resolvedPath, 1) === CHAR_BACKWARD_SLASH) {
	const code = StringPrototypeCharCodeAt(resolvedPath, 2);
	if (code !== CHAR_QUESTION_MARK && code !== CHAR_DOT) {
	// Matched non-long UNC root, convert the path to a long UNC path
	return `\\\\?\\UNC\\${StringPrototypeSlice(resolvedPath, 2)}`;
	}
	}
	} else if (
	isWindowsDeviceRoot(StringPrototypeCharCodeAt(resolvedPath, 0)) &&
	StringPrototypeCharCodeAt(resolvedPath, 1) === CHAR_COLON &&
	StringPrototypeCharCodeAt(resolvedPath, 2) === CHAR_BACKWARD_SLASH
	) {
	// Matched device root, convert the path to a long UNC path
	return `\\\\?\\${resolvedPath}`;
	}

Uh oh!

Conversation

StefanStojanovic commented Dec 8, 2023

Uh oh!

anonrig left a comment

Choose a reason for hiding this comment

Uh oh!

richardlau commented Dec 8, 2023

Uh oh!

StefanStojanovic commented Dec 8, 2023

Uh oh!

StefanStojanovic commented Dec 15, 2023

Uh oh!

joyeecheung left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StefanStojanovic commented Feb 8, 2024

Uh oh!

joyeecheung commented Feb 8, 2024

Uh oh!

StefanStojanovic commented Feb 9, 2024

Uh oh!

nodejs-github-bot commented Feb 20, 2024

Uh oh!

nodejs-github-bot commented Feb 29, 2024

Uh oh!

nodejs-github-bot commented Feb 29, 2024

Uh oh!

nodejs-github-bot commented Mar 1, 2024

Uh oh!

nodejs-github-bot commented Mar 19, 2024

Uh oh!

StefanStojanovic commented Apr 1, 2024

Uh oh!

nodejs-github-bot commented Apr 1, 2024

Uh oh!

karlhorky commented Apr 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 8, 2024

Uh oh!

StefanStojanovic commented Apr 8, 2024

Uh oh!

nodejs-github-bot commented Apr 8, 2024

Uh oh!

nodejs-github-bot commented Apr 8, 2024

Uh oh!

karlhorky commented May 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

joyeecheung left a comment •

edited

Loading

karlhorky commented Apr 3, 2024 •

edited

Loading